Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding support for Scalar Crypto Extension (Bitmanip instructions for Cryptography, Zbkb) #2653

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Adding support for Scalar Crypto Extension (Bitmanip instructions for Cryptography, Zbkb) #2653

wants to merge 13 commits into from
+297 −21

Conversation

munailwaqar
Copy link

Introduction

This PR adds support for Zbkb extension in the CVA6 core. It also adds the documentation for this extension. These changes have been tested with self-written single instruction tests and with the riscv-arch-tests. This PR will be followed by other PRs that will add complete support for the Zkn - NIST Algorithm Suite extension.

Implementation

Zbkb Extension:
Added support for the Zbkb instruction set. It essentially expands the Zbb extension with additional instructions useful in cryptography. These instructions are pack, packh, packw, brev8, unzip and zip.

Modifications

  1. A new bit ZKN was added. The complete Zkn extension will be added under this bit for ease of use. This configuration will also require the RVB (bitmanip) bit to be set.
  2. Updated the ALU and decoder to recognize and handle Zbkb instructions.

Documentation and Reference

The official RISC-V Cryptography Extensions Volume I was followed to ensure alignment with ratification. The relevant documentation for the Zbkb instruction was also added.

Verification

Assembly Tests:
The instructions were tested and verified with the K module of both 32 bit and 64 bit versions of the riscv-arch-tests to ensure proper functionality. These tests check for ISA compliance, edge cases and use assertions to ensure expected behavior. The tests include:

  1. pack-01.S
  2. packh-01.S
  3. packw-01.S
  4. brev8-01.S
  5. unzip-01.S
  6. zip-01.S

github-actions[bot]
github-actions bot reviewed Dec 5, 2024
View reviewed changes
core/alu.sv Outdated Show resolved Hide resolved
core/alu.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
@JeanRochCoulon
Copy link
Contributor

Great! To solve the Verible format issues, I suggest to use the command reported in CONTRIBUTING.md.
Do not forget to sign the ECA agreement to bcome cva6 contributor.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2024

❌ failed run, report available here.

@fatimasaleem fatimasaleem self-requested a review December 5, 2024 13:21
@JeanRochCoulon
Copy link
Contributor

First comments:

  • cv32a60x and cv32a65x do not need the new extension. I suggest to apply the extension the configuration used for FPGA: cv32a6_imac_sv32 and disable the extension for 60x and 65x.
  • In config_pkg.sv files, better is to not define localparam. You can follow the 65x_config_pkg.sv example.
    Cheers

@fatimasaleem
Copy link
Contributor

First comments:

  • cv32a60x and cv32a65x do not need the new extension. I suggest to apply the extension the configuration used for FPGA: cv32a6_imac_sv32 and disable the extension for 60x and 65x.
  • In config_pkg.sv files, better is to not define localparam. You can follow the 65x_config_pkg.sv example.
    Cheers

Thanks @JeanRochCoulon . Sure, we will disable it for cv32a60x and cv32a65x . We only wanted to run the CI with this extension enabled. Since now the CI is almost green, we will disable it for these two.

github-actions[bot]
github-actions bot reviewed Dec 9, 2024
View reviewed changes
core/alu.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
core/decoder.sv Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

❌ failed run, report available here.

JeanRochCoulon
JeanRochCoulon reviewed Dec 9, 2024
View reviewed changes
verif/sim/cva6.py Show resolved Hide resolved
verif/sim/cva6.py Show resolved Hide resolved
@munailwaqar
Copy link
Author

Currently enabling Zbkb in 2 configs only:

  1. cv32a6_imac_sv32
  2. cv64a6_imafdc_sv39

@github-actions GitHub Actions
Copy link
Contributor

❌ failed run, report available here.

github-actions[bot]
github-actions bot reviewed Dec 10, 2024
View reviewed changes
core/decoder.sv Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

❌ failed run, report available here.

@github-actions GitHub Actions
Copy link
Contributor

✔️ successful run, report available here.

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

✔️ successful run, report available here.

@fatimasaleem
Copy link
Contributor

Hi @JeanRochCoulon, can you please approve this?

@JeanRochCoulon
Copy link
Contributor

Hello @munailwaqar
Great to have identified riscv-arc-test tests to test the new extension. Can you add these test to the testlist of the 64bit configuration (cv64a6_imafdc_sv39) to insert them into CI and ease the maintenance.

@github-actions GitHub Actions
Copy link
Contributor

✔️ successful run, report available here.

@munailwaqar
Copy link
Author

munailwaqar commented Dec 12, 2024 •
edited
Loading

Hello @munailwaqar Great to have identified riscv-arc-test tests to test the new extension. Can you add these test to the testlist of the 64bit configuration (cv64a6_imafdc_sv39) to insert them into CI and ease the maintenance.

@JeanRochCoulon Added the relevant arch tests to the testlist but I also had to enable the Zbkb extension in cv64a6_imafdc_sv39_hpdcache config as the CI runs checks for this config too using the same testlist.

@github-actions GitHub Actions
Copy link
Contributor

✔️ successful run, report available here.

@fatimasaleem
Copy link
Contributor

Hi @JeanRochCoulon can you please review this again? Thanks.

@JeanRochCoulon
Copy link
Contributor

Hello @munailwaqar
zip and unzip tests are not included in testlist, is it intentional ?

@munailwaqar
Copy link
Author

Hello @munailwaqar zip and unzip tests are not included in testlist, is it intentional ?

@JeanRochCoulon Yes. zip and unzip instructions are only for 32bit architectures according to the documentation.

@JeanRochCoulon
Copy link
Contributor

I cannot rebase because I have no right on your branch, can you ?

@munailwaqar
Copy link
Author

munailwaqar commented Dec 17, 2024 via email

@munailwaqar
Copy link
Author

@JeanRochCoulon I have rebased the branch.

@github-actions GitHub Actions
Copy link
Contributor

❌ failed run, report available here.

@JeanRochCoulon
Copy link
Contributor

LGTM. I am ready to merge.
Can you rebase (again) then, taking back the resulting gate count, adjust the expected gate count ?

@github-actions GitHub Actions
Copy link
Contributor

❌ failed run, report available here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanRochCoulon JeanRochCoulon JeanRochCoulon left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@jquevremont jquevremont Awaiting requested review from jquevremont jquevremont is a code owner

@zarubaf zarubaf Awaiting requested review from zarubaf zarubaf is a code owner

@fatimasaleem fatimasaleem Awaiting requested review from fatimasaleem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@munailwaqar @JeanRochCoulon @fatimasaleem